refactor(network): use async for registry network operations#16745
refactor(network): use async for registry network operations#16745arlosi wants to merge 6 commits intorust-lang:masterfrom
Conversation
5072ba5 to
e09814c
Compare
This comment has been minimized.
This comment has been minimized.
4e38864 to
a462b06
Compare
|
@rustbot author Splitting this out into a few commits to make it easier to review. Current outstanding question is whether to use |
|
Reminder, once the PR becomes ready for a review, use |
Avoid making trait objects from the `Registry` trait and the associated dynamic dispatch. The `Registry` trait only has two implementations `PackageRegistry` and `MyRegistry`. `MyRegistry` is only used for tests. This change came out of #16745 where query functions are changed to `async`. Dynamic dispatch on async functions requires boxing. The other two key registry-related traits ( `RegistryData` and `Source`) *do* have multiple implementations and cannot be easily changed like `Registry` here.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
26e5fb2 to
5442223
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready I've split the PR into several commits. The last one is still large, but it's about 1/2 the size. |
This comment has been minimized.
This comment has been minimized.
|
I've split the commits up further, though the last commit is still pretty large. A few of the very small ones could probably go in their own PR independent of this one. The commits that add functionality could go in their own PRs, but nothing would use them until later - such as the async http client. |
This comment has been minimized.
This comment has been minimized.
…unction; implementations are not changed
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
What does this PR try to resolve?
The existing model for registry index operations uses
Poll+block_until_readyto enable parallelism. This model is somewhat awkward to work with, especially when parallelism is desired. Usingasyncsimplifies creating and working with registries.Convert registry methods taking
&mut selfto&selfIn order to execute multiple futures in parallel (using
FuturesUnorderedor similar), there must be multiple active borrows. Many of the registry querying methods used&mut self, which prevents this.Any mutations are now handled by using
Cell/RefCell. Since the executor is single-threaded, the operations aren't actually happening in parallel, but when passing an.awaitpoint, a different future could begin executing. We must be careful now not to hold mut borrows onRefCells across await points, as it will cause a panic.Convert methods returning
Poll<_>to returnFuture<_>(async).When we want to block (and we're not in an async context), the async call is wrapped with
futures::executor::block_on. If this is attempted within an existing async executor, cargo will panic.Remove all
block_until_readymethods from allSource,RegistryDatatraitsFor most of the sources, the contents of the
block_until_readyfunction was moved into a a privateupdatefunction that's called once before making any queries.Add new HTTP async
http_asyncmoduleWraps cURL's
Multiinterface to provide anasyncinterface for making HTTP requests through cURL.Add
async-traitcrateThis is used to enable dynamic dispatch on traits that use
async fn.Error message output changes
Due to refactoring around removing
block_until_ready, some error messages are changed, usually by having additional context information. This change makes the error messages more consistent between various code paths. I attempted to minimize the changes in this PR for ease of review. We may want to simplify the error messages (by removing one level of context) in a follow up.Resolver
The resolver is the only portion still using the
Pollapproach. An adapter layerLocalPollAdapteris added that translatesPolls intoasync.Converting the resolver be
asyncnative is left as future work. In theory this conversion could provide performance improvements, since the resolution would not need to be restarted. It would also let the resolver start working immediately as new dependencies became available..cratedownloadingThe downloading of crate files using
PackageSetis not yet converted to async. This is expected to be straightforward to do in a separate PR.Progress reporting
The
http_remoteno longer has the ability to track how manyblock_until_readycalls have been made, so we can't do the trick where we assume we have a dependency tree depth of 10 to make a fake progress bar. Since the progress bar was fake anyway, this change removes it in favor of only displaying the information we know.The new behavior is:
Performance
This change is not expected to impact performance. My local measurements show neutral to possible 1% improvement. Release binary size increased about 1%.
How to review
Commit by commit. The final commit contains the majority of the change that was not straightforward to separate out.